feat(helm): machine-a-tron mac address allocation#3700
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Summary by CodeRabbit
WalkthroughThe Helm chart adds deterministic, configurable per-pod MAC pools and hardware MAC ranges. Values, templates, multi-pod examples, and deployment documentation now describe automatic allocation and explicit override paths. ChangesPer-pod MAC pool allocation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant ConfigMapTemplate
participant MatToml
HelmValues->>ConfigMapTemplate: Supply MAC defaults and pod overrides
ConfigMapTemplate->>ConfigMapTemplate: Sort active pods and calculate podIndex
ConfigMapTemplate->>MatToml: Render per-pod MAC pool sections
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@helm/charts/nico-machine-a-tron/README.md`:
- Around line 47-51: Restore the TOML code-fence boundaries in
helm/charts/nico-machine-a-tron/README.md: at lines 47-51, open a toml fence
before [site_explorer] and close it after override_target_port; at lines
235-236, open a toml fence before [networks.pod-0-oob] and close it after the
final gateway. Ensure subsequent Markdown sections render normally.
- Around line 367-375: Update the troubleshooting section in the README by
changing the “Check service selector” and “Check pod labels” headings to
lower-level headings that remain nested under the parent section. Wrap each
kubectl command in its own closed bash code fence, keeping the existing commands
and making the Markdown structure render correctly.
In `@helm/charts/nico-machine-a-tron/templates/configmap.yaml`:
- Line 36: Update the $hwMacBasePrefix fallback in the ConfigMap template from
the single-octet "06" value to the documented two-octet default "02:01",
matching values.yaml and producing valid hardware address prefixes when
basePrefix is absent.
- Around line 54-60: Validate the computed $podIndex after the podList lookup
and fail template rendering when it is greater than or equal to 256, before the
mac_address_pool and hw_mac_address_ranges values format it with %02x. Preserve
the existing index calculation for valid indices and use the chart’s established
template failure mechanism.
- Around line 29-35: Update the macAddressPool and hwMacAddressRanges generation
toggle assignments, including $macPoolEnabled and $hwMacRangesEnabled, so
explicit false values are preserved instead of being replaced by default true.
Use dig with an enabled key default or an explicit key-presence check, while
retaining true as the fallback when the key is absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 40c58f70-2d80-4a00-a75e-ab3bba7f42e5
📒 Files selected for processing (5)
docs/development/machine-a-tron-deployment.mdhelm-prereqs/values/machine-a-tron-multipod.yamlhelm/charts/nico-machine-a-tron/README.mdhelm/charts/nico-machine-a-tron/templates/configmap.yamlhelm/charts/nico-machine-a-tron/values.yaml
|
🌿 Preview your docs: https://nvidia-preview-pull-request-3700.docs.buildwithfern.com/infra-controller |
94eb19c to
ce37eff
Compare
ce37eff to
536e79d
Compare
shayan1995
left a comment
There was a problem hiding this comment.
Thanks — this is a real quality-of-life improvement over the full per-pod TOML overrides, and I verified the template logic renders correctly (skip conditions consistent between the count/list/render loops, per-pod override precedence, enabled: false, the 256-pod guard, and the multipod example matching its comments). Two issues worth addressing before merge, left inline.
shayan1995
left a comment
There was a problem hiding this comment.
left two comments to consider but generally it looks good.
shayan1995
left a comment
There was a problem hiding this comment.
Open concerns addressed — #3814 filed for the pod-index stability edge case. LGTM.
Adds auto-generated per-pod MAC address pools to prevent collisions in multi-pod machine-a-tron deployments.
mac_address_pool- Standalone MACs (BMC, DPU interfaces)hw_mac_address_ranges- Contiguous NIC blocks (ConnectX, storage)Related issues
Closes #3698
Type of Change
Breaking Changes
Testing
Test Results